TO-404: remove solution-specific fields - #835
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements TO-404 by moving solution-specific artefact metadata into a generic attributes JSONB column, removing the bundled-builds schema and API surface, and updating solution uniqueness to be based on (name, version).
Changes:
- Add
Artefact.attributes(JSONB) and expose it via artefact GET/PATCH and solution start-test requests. - Remove bundled-builds model/API (
bundled_builds_hash, association table, and related response fields). - Change solution uniqueness constraint to
(name, version)and update tests/migration coverage accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py | Adds migration tests covering copying bundled-build fields into attributes and schema upgrade/downgrade behavior. |
| backend/tests/data_generator.py | Updates test data generator to support creating artefacts with attributes. |
| backend/tests/data_access/test_models.py | Adds tests for the new solution uniqueness constraint semantics. |
| backend/tests/controllers/test_executions/test_start_test.py | Updates solution start-test behavior/tests for attributes and new uniqueness rules. |
| backend/tests/controllers/test_executions/test_reruns.py | Updates rerun-related expected artefact payload to include attributes and drop bundled-builds. |
| backend/tests/controllers/auth/test_saml.py | Adjusts IdP public netloc port used by SAML tests. |
| backend/tests/controllers/artefacts/test_builds.py | Updates artefact build response expectations after removing bundled-builds references. |
| backend/tests/controllers/artefacts/test_artefacts.py | Adds tests for attributes in artefact GET/PATCH; removes bundled-builds tests and response expectations. |
| backend/test_observer/data_access/repository.py | Removes eager-loading of bundled-builds from artefact listing query paths. |
| backend/test_observer/data_access/models.py | Replaces bundled-builds schema/relationships with a JSONB attributes field and updates solution unique index. |
| backend/test_observer/controllers/test_executions/start_test.py | Stops using bundled-builds hash for solutions; stores solution attributes on artefact creation. |
| backend/test_observer/controllers/test_executions/models.py | Replaces solution track/source request fields with attributes; updates stage description. |
| backend/test_observer/controllers/artefacts/models.py | Adds attributes to artefact responses and patch model; removes bundled-build fields/models. |
| backend/test_observer/controllers/artefacts/builds.py | Removes bundled-in eager-loading from build retrieval. |
| backend/test_observer/controllers/artefacts/artefacts.py | Removes bundled-build handling from artefact list/get/patch/history and versions query. |
| backend/schemata/openapi.json | Updates OpenAPI schema to reflect new attributes field and removed bundled-builds fields. |
| backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py | Adds attributes, changes unique_solution, migrates bundled-build data into attributes, and drops legacy schema. |
Comments suppressed due to low confidence (1)
backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:46
- This migration both introduces the new
attributescolumn and drops the legacybundled_builds_hashcolumn andartefact_bundled_builds_associationtable in the same deploy step. In rolling/HA deployments, this can break older application instances still expecting the old schema (expand/contract guideline).
Consider splitting into at least two migrations/PRs: (1) add & backfill attributes while keeping old columns/tables, deploy code that can read/write the new field, then (2) remove the legacy columns/tables in a later deploy.
def _remove_bundled_builds() -> None:
_copy_bundled_builds_to_attributes()
op.drop_table("artefact_bundled_builds_association")
op.drop_column("artefact", "bundled_builds_hash")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:26
- The new unique index on solutions is only (name, version). If the existing database contains multiple solution rows with the same name/version (previously allowed by including source/track/stage/bundled_builds_hash),
op.create_index(..., unique=True)will fail during upgrade and block deployment. The migration should either reconcile duplicates (e.g. merge/archive) or proactively detect and raise a clear, actionable error before attempting to create the index.
def upgrade() -> None:
op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False))
op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)")
op.create_index(
"unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'")
)
_remove_bundled_builds()
backend/test_observer/data_access/models.py:328
- The comment suggests
attributesis solution-specific, but the column is defined on the genericArtefactmodel and is returned by the generic artefact API response. This comment is misleading and makes the field look like a violation of the project’s family-agnostic design, even though the implementation is generic.
# (for now) Solution specific field
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
backend/test_observer/controllers/artefacts/models.py:94
- The deprecated
bundled_buildsfield now returnslist[Any], which downgrades the OpenAPI schema (items become untyped{}) and is less backwards-compatible for clients that still use the old response shape. Since it is always empty, you can keep the previous element type to preserve the contract while still deprecating it.
@computed_field(
deprecated="bundled_builds is deprecated and always empty; solutions now use the generic "
"attributes field instead.",
)
def bundled_builds(self) -> list[Any]:
| def _remove_bundled_builds() -> None: | ||
| _copy_bundled_builds_to_attributes() | ||
| op.drop_table("artefact_bundled_builds_association") | ||
| op.drop_column("artefact", "bundled_builds_hash") | ||
|
|
There was a problem hiding this comment.
For any reviewer, please see migration tests
There was a problem hiding this comment.
I added the Copilot instructions to recommend the expand-contract pattern. The issue is in an HA deployment, the upgrades can happen at different times, so one unit might still be using old code with the old schema while the migration has already changed the database to use the new schema, or vice versa, and this can cause errors.
I think this should still be split, possibly into three migrations
- The first migration adds the
artefact.attributescolumn - The second is a data-only migration that copies bundled builds to attributes for solutions
- The third changes the index and drops
artefact.bundled_builds_hashandartefact_bundled_builds_association
There was a problem hiding this comment.
I updated this PR with the split, I'll make changes based on the next copilot review before splitting into two PRs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:42
- This migration both expands (adds
artefact.attributes+ new unique index) and contracts (dropsartefact_bundled_builds_associationandartefact.bundled_builds_hash) in the same upgrade. In rolling/HA deployments, applying the migration while old app versions are still running can break them (they may still read/write the dropped table/column). Consider splitting into an expand migration (add attributes + copy data + keep old table/column) and a later contract migration (drop old structures) after the new code is fully deployed.
def upgrade() -> None:
op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False))
_assert_no_duplicate_solutions(["name", "version"])
op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)")
op.create_index(
"unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'")
)
_remove_bundled_builds()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
backend/test_observer/data_access/models.py:380
unique_solutionis updated in the ORM metadata to only cover (name, version), but the migrations in this PR do not swap the database index accordingly (the new migration is explicitly an expand step that leaves the legacyunique_solutionindex in place). This schema drift can both breakalembic check/autogenerate and leave production DBs upgraded via migrations enforcing a different uniqueness rule than the ORM/runtime code assumes.
Either keep the legacy index/columns represented in Base.metadata until the contract migration is applied, or add the contract migration in this PR (swap unique_solution in the DB, then drop the legacy column/table only after rollout).
Index(
"unique_solution",
"name",
"version",
postgresql_where=column("family") == FamilyName.solution.name,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
backend/test_observer/controllers/test_executions/start_test.py:353
- For solutions,
create_artefact()now callsget_or_create()withfilter_kwargslimited to (name, version, family). In a database that already contains multiple solution rows for the same name/version (possible under the currentunique_solutiondefinition, and during rollout),get_or_create()will return an arbitrary match (.first()), so new test executions may attach to the wrong artefact. Either enforce the new (name, version) uniqueness at the DB level before using this lookup, or make the lookup deterministic / detect duplicates and fail fast.
case StartSolutionTestExecutionRequest():
creation_kwargs["attributes"] = self.request.attributes
self.artefact = get_or_create(self.db, Artefact, filter_kwargs=filter_kwargs, creation_kwargs=creation_kwargs)
backend/test_observer/data_access/models.py:380
Artefactmodel metadata now definesunique_solutionas unique on (name, version) only, but the only migration in this PR (2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py) does not recreateunique_solution(and explicitly documents keeping the old schema for rolling upgrades). This mismatch will makealembic check/ autogenerate report pending schema changes, and it undermines the “expand”/rolling-upgrade contract described by the migration tests.
Index(
"unique_solution",
"name",
"version",
postgresql_where=column("family") == FamilyName.solution.name,
…n-specific-fields
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backend/test_observer/controllers/test_executions/models.py:324
map_legacy_track_and_sourcemutates the incomingattributesdict (and thedatadict) in-place. Because this is amode="before"validator, the input object may be reused by the caller (e.g., unit tests or internal Python callers), and in-place mutation can lead to surprising side effects. Safer to copyattributes(and return a newdatadict) before adding legacytrack/source.
attributes = data["attributes"]
if attributes is None or not isinstance(attributes, dict):
return data
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
backend/migrations/env.py:25
typing.Tupleis only used for_EXPAND_CONTRACT_IGNORED_COLUMNS. Prefer the builtintuple(PEP 585) and drop the extra import (this also avoids import-order/isort complaints).
# for 'autogenerate' support
target_metadata = Base.metadata
backend/tests/data_generator.py:168
attributes = attributes or {}treats an explicitly provided empty dict as "not provided" and replaces it with a new dict. Use an explicit None check so callers passing{}keep their object/value unchanged.
created_at = created_at or datetime.utcnow()
reviewers = reviewers or []
attributes = attributes or {}
backend/migrations/env.py:52
- The
include_objectcallback parameter nameobjectshadows the Python builtinobjectand may be flagged by Ruff/flake8-builtins. Renaming it also makes it clearer this is an Alembic/SQLAlchemy object.
return False
if type_ == "column":
table_name = object.table.name if object.table is not None else None
if (table_name, name) in _EXPAND_CONTRACT_IGNORED_COLUMNS:
return False
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (2)
backend/test_observer/controllers/artefacts/models.py:75
- Removing the previously-exposed
bundled_buildsfield from the/v1/artefacts/*response is a backward-incompatible API change for existing clients. If clients still depend on this field, consider either (a) keeping it temporarily as a deprecated field (e.g. defaulting to an empty list) or (b) introducing a versioned response (/v2) for the breaking change.
stage: str
family: str
status: ArtefactStatus
comment: str
attributes: dict[str, Any]
archived: bool
backend/test_observer/controllers/artefacts/models.py:151
- Removing the
bundled_infield fromArtefactBuildResponseis a backward-incompatible change for clients consuming build details. If this field has active consumers, consider keeping it temporarily as a deprecated field (even if it always returns[]) or moving the change behind a versioned API response.
model_config = ConfigDict(from_attributes=True)
id: int
architecture: str
revision: int | None
test_executions: list[TestExecutionResponse]
Description
Resolved issues
Resolves TO-404
Documentation
Web service API changes
Updated the API accordingly
Tests
There are new unit tests